-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable REPL to offer to install missing packages if install hooks are provided #39026
Enable REPL to offer to install missing packages if install hooks are provided #39026
Conversation
a6aecac
to
29ec192
Compare
Nice! I do think that Pkg itself needs to manage some of this though (and not only REPL). Some comments:
|
Strongly agreed. It would be annoying to get this if you had made a typo in the package name.
I think that can be a seperate PR? |
Ok, I've made some changes:
Demo: https://asciinema.org/a/adqDA5jSp3fusjSdIPOtttvqK
|
This is only in interactive usage, right? I.e. in scripts, you'd just throw the error. |
Yeah. That's the intention. This is what I've tested
|
Just pushed to the corresponding pkg PR to improve the info and formatting of the prompt: |
I like this! However, do we need to force the user into a yes/no dialog? These can be rather annoying. What about a more minimal approach, something like julia> using CSV
Package "CSV" not found, but available from a registry.
Install the package with `] add CSV` or by pressing `CTRL+I`.
julia> Which would install the package upon pressing |
One option is to make the default That would feel less like forcing the user into a decision. Providing a magic key combo to run the package add is also interesting |
The yes/no dialog is what most package managers do here, so it seems pretty standard and easy. |
Any further thoughts on this? In summary I think the flow works well, and it seems like it would allow exotic front ends to do whatever it prefers when the However, I think there's one (hopefully small) con with this strategy of intercepting errors in the REPL evaluation..
then or
|
38fb0f1
to
149afd8
Compare
stdlib/REPL/src/REPL.jl
Outdated
function handle_install_package_hooks(mod, ast, backend) | ||
line = string(ast.args[2]) | ||
# only exercise hooks for lines starting with `using/import` to avoid re-running non-load code | ||
if startswith(line, "using") || startswith(line, "import") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the best way to check this, since there could also be leading whitespace or the using
statement could be further nested but still run first. We could do some analysis on the parsed AST, but I am wondering if in practice just always reevaluating the whole input would really be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I am wondering if in practice just always reevaluating the whole input would really be a problem.
Yeah, I think you might be right. I've removed the check for using/import
Feature request: if a user does using Foo, Bar It will prompt to install all of them. As Stefan says, if users want an in between, they can do it manually. |
Suggestion from Stefan: After the repl input is parsed, search for |
The advantage of Stefan's parse-first method is it avoids all issues with when and how many times things are evaluated. Even if the input contains A "disadvantage" (but depends on your point of view) is that meta-programmed |
c68f689
to
ba8e166
Compare
Now updated along with JuliaLang/Pkg.jl#2307 to @StefanKarpinski's suggestion of checking for all packages that will be loaded in the ast, then handling all packages in one act. It seems to work quite nicely. A single package
Where all packages are missing, but only two packages are available in a registry
Where specific imports are declared
In guarded code
The first examplle but selecting
The second example but selecting
What it actually looks like on MacOS Doesn't add noticeable overhead to REPL eval |
Beautiful! |
e70611a
to
086a556
Compare
stdlib/REPL/src/REPL.jl
Outdated
if first(arg.args) isa Symbol # i.e. `Foo` | ||
push!(mods, first(arg.args)) | ||
else # i.e. `Foo: bar` | ||
push!(mods, first(first(arg.args).args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might need to be made more robust. I don't think this works for something like import A.b as c
yet. Does this currently know to properly ignore imports like using .A
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think about either of those. I'll investigate and add tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the using .A
case, and the other worked already. Added tests. Are there any other forms to watch out for?
stdlib/REPL/src/REPL.jl
Outdated
function eval_with_backend(ast, backend::REPLBackendRef) | ||
if !isempty(install_packages_hooks) | ||
check_for_missing_packages_and_run_hooks(ast) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably run in the backend. It could even use the existing repl_ast_transforms
hook (not a "transform" per se, but no big deal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Moved to the backend, but given that the modules_to_be_loaded
parser resides here, it seemed like a misuse to place that on repl_ast_transforms
given that if IJulia/Pluto etc. wanted to do a different install hook they'd probably want to go after the modules have been identified by modules_to_be_loaded
so keeping install_packages_hooks
would be necessary anyway. These use models are a bit imaginary though.. my thinking might be off
For those following this, there's some unresolved discussion about the message formatting in JuliaLang/Pkg.jl#2307 |
Just trying out the ideas discussed in #26469 and in #35062 and the suggestion here #26469 (comment) to introduce a specific error type to allow other handlers to decide what to do.
This goes for a more basic
y/n
prompt than the example in #26469, with the default beingn
, which is selected here first.It also leaves it up to the user to decide if the active environment is appropriate to install into.
A
ctrl-c
acts the same as returningn
or just pressingenter
.Here the default is selected first by just hitting
enter
cc. @KristofferC @oxinabox